-
Notifications
You must be signed in to change notification settings - Fork 182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UI: Adding Bounding Box & Fixing Alignment issue in TextBlock2D #803
Conversation
Hi @ganimtron-10, What is the status of this PR? Many tests are failing! I suppose that you might need to record again many tests because of this change. Before doing that, let me know when I can check and test this PR |
Sure @skoudoro will let you know, Currently working on the tests itself. |
Hey @ganimtron-10 is this ready for review or should we wait for you to ping us again? I see that you made some commits yesterday, so I don't know whether I should start or wait. |
Hey @JoaoDell you can go ahead! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ganimtron-10, I have just finished my first review here and, overall, it seems fine and working. I have ran the ui test and everything passed, so it does not seem to have any problem. I would just like to know if you recommend any example to be ran so I can check the new features and changes visually. Nice work!
Hey @JoaoDell , You can try out this below code to try out the feature visually. from fury import ui, window
from fury.data import fetch_viz_icons
fetch_viz_icons()
c = ui.TextBox2D(width=12, height=3, justification='center') #left, right, center
show_manager = window.ShowManager(size=(500, 500),
title="FURY textbox Example")
show_manager.scene.add(c)
show_manager.start() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ganimtron-10, I think it is looking good, you have made great progress. I left a few comments and questions below for you to check them. And I have another question related to the two enhancements mentioned.
Between auto_font_scale
and dynamic_bbox
, which one has the priority (what happens when I set both true)? because both seem to be opposite as with_auto_font_scale_, the bounding box is the one which set the size of the text, but with dynamic_bbox, it is the text size that sets the bounding box size. Did I understand correctly?
Both of the parameter can co exist! |
Thanks @JoaoDell @tvcastillod for the review! |
Hi @ganimtron-10, Tests are still failing, do you plan to look at them ? please, look at tabui |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, it looks good.
The code is better like that, it is much simpler. I still need to play a bit more.
see below few comments that can be fixed quickly
Thanks @ganimtron-10
Hey @skoudoro ,
I checked those last time and it worked fine! Maybe I saw outdated version, My bad. |
I was also wondering do we need |
Codecov Report
@@ Coverage Diff @@
## master #803 +/- ##
==========================================
- Coverage 84.41% 84.30% -0.12%
==========================================
Files 43 44 +1
Lines 10166 10353 +187
Branches 1381 1406 +25
==========================================
+ Hits 8582 8728 +146
- Misses 1227 1255 +28
- Partials 357 370 +13
|
Hey @skoudoro , |
Great! let me play a bit with this and we can move forward |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @ganimtron-10! Let's move forward. merging
The
TextBlock2D
component in the UI system was encountering alignment and scaling issues. When resizing theTextBlock2D
, the text scaling mode was set to "SetTextScaleModeToProp," which resulted in inconsistent alignment of the text actor and improper justification of the text with respect to the background rectangle. Additionally, the background rectangle's size was calculated only after it was added to the scene, which is not an ideal practice.To address these issues, this PR introduces the following enhancements:
BoundingBox Property: A new property called "boundingbox" is implemented in the
TextBlock2D
component. This property calculates the text bounding box based on the content of theTextBlock2D
. It ensures accurate alignment and justification of the text within the component.Auto Font Scale: The scaling mode is now separated from the resizing action. A new property named "auto_font_scale" is introduced, which, when set to True, enables automatic font scaling according to the bounding box. This allows for consistent and proportional text sizing without manual intervention.
By implementing these changes, the
TextBlock2D
component will provide improved text alignment, justified text with the background rectangle, and a more intuitive font scaling experience.